Connector & Engine adjustments for asset scale precision loss leftovers by gakonst · Pull Request #199 · interledger/interledger-rs

gakonst · 2019-08-08T10:45:25Z

Closes #185

gakonst · 2019-08-09T09:31:19Z

Adding a type_length_limit check at the main.rs file does not seem to allow tests to build:

cargo build --all-features --all-targets
   Compiling interledger-settlement-engines v0.1.0 (/Users/gakonst/projects/xpring-contract/interledger-rs/crates/interledger-settlement-engines)
error: reached the type-length limit while instantiating `<std::vec::IntoIter<ethereum_typ...]]], std::result::Result<(), !>>`
     |
     = note: consider adding a `#![type_length_limit="1689024"]` attribute to your crate

error: aborting due to previous error

error: Could not compile `interledger-settlement-engines`.
warning: build failed, waiting for other jobs to finish...
error: reached the type-length limit while instantiating `<std::vec::IntoIter<ethereum_typ...]]], std::result::Result<(), !>>`
     |
     = note: consider adding a `#![type_length_limit="1689024"]` attribute to your crate

error: aborting due to previous error

error: Could not compile `interledger-settlement-engines`.
warning: build failed, waiting for other jobs to finish...
error: reached the type-length limit while instantiating `<std::vec::IntoIter<ethereum_typ...]]], std::result::Result<(), !>>`
     |
     = note: consider adding a `#![type_length_limit="1689024"]` attribute to your crate

error: aborting due to previous error

error: Could not compile `interledger-settlement-engines`.

Error is pretty opaque, does not indicate where we need to adjust things

Implement the trait for the eth engine

This allows the trait implementer to choose the data type of the leftovers instead of forcing them to use BigUint

gakonst · 2019-08-12T12:07:58Z

Maybe related: rust-lang/rust#54540 (comment)

Turns out boxing the futures instead of returning Either's does the trick. Not sure how I feel about it, but I cannot find another workaround.

In linkmauve/tokio-socks5@a532d63, they allude how they use Boxes to avoid long compilation times because the nested futures really make the compiler tired.

Note: https://github.com/interledgerjs/settlement-xrp is not updated yet to return data for checking leftovers

gakonst · 2019-08-12T14:05:26Z

@emschwartz this should be good for review again. Curious what you think on Box'ing the futures. Seems like there's no good alternative.

emschwartz

I think I still prefer load_uncredited_settlement_amount and save_uncredited_settlement_amount over load/pop leftovers

emschwartz · 2019-08-12T15:13:01Z

+    ) -> Box<dyn Future<Item = (), Error = ()> + Send> {
+        trace!("Saving leftovers {:?} {:?}", account_id, leftovers);
+        let mut pipe = redis::pipe();
+        pipe.set(format!("leftovers:{}", account_id), leftovers.to_string())


This should be adding to that amount rather than overwriting what's there

Yeah that's correct since there may be a race condition. However, the amount being written is a BigNumber, we cannot do arithmetic with these in the store, unless we store them as numbers in redis.

What should we do if the connector replies with leftovers greater than u64? Alternatively, we should explore some way to perform atomic threadsafe operations on Redis where we pull some value, operate on it and write back to it.

Kind of a janky solution but we could use a list of strings in Redis. When you want to save an amount, you would add another number to the list. When you want to get the leftovers, you would load the whole list (and empty it out -- although this creates a possibility that information would get lost if the SE crashes right then) and sum up the numbers parsed from the strings using the big integer library.

I like the list of strings idea. When we fetch the leftovers, I also think we can mitigate any risk of the SE crashing:

Generate a new idempotency key locally

Run a pipeline or Lua script that:
a. Gets the uncredited list (and returns right away if it's empty)
b. Sets new key(s) with the idempotency key, its expiration, and the list of amounts it corresponds to
b. Empties the uncredited list
d. Returns the list of amounts corresponding to the idempotency key

Then, the BigNumber logic locally can sum up the list of amounts for the incoming settlement request. If it crashes and comes back online, it can safely lookup all unexpired idempotency keys, and add up their list of amounts locally to compute the total amount that should be sent in each of the incoming settlement requests.

If that idempotency key expires/the request fails after a few retries, we can atomically append its list of amounts to the list of uncredited amounts and delete the idempotency key.

The list of strings logic has been added, thanks for the suggestion @emschwartz.

emschwartz · 2019-08-13T15:16:55Z

-                            Ok(())
-                        })
-                })
+            let amount = BigUint::from(100u64);


This test should use numbers that are larger than u64, just to ensure it does exactly what we expect

… u64

emschwartz · 2019-08-14T14:35:12Z

+            uncredited_settlement_amount
+        );
+        let mut pipe = redis::pipe();
+        pipe.lpush(


Might be worth including a comment recording why these amounts are stored as a list

emschwartz · 2019-08-14T14:36:04Z

+        let mut pipe = redis::pipe();
+        // Loads the value and resets it to 0
+        pipe.lrange(
+            format!("uncredited_settlement_amount:{}", account_id),


Should this key be prefixed with the SE type like the other keys?

gakonst requested a review from emschwartz as a code owner August 8, 2019 10:45

gakonst changed the base branch from master to gakonst-asset-scale August 8, 2019 10:45

gakonst changed the title ~~Connector & Engine adjustments for precision loss leftovers~~ Connector & Engine adjustments for asset scale precision loss leftovers Aug 8, 2019

emschwartz reviewed Aug 8, 2019

View reviewed changes

gakonst mentioned this pull request Aug 8, 2019

Improve Asset Scale conversions #192

Merged

gakonst force-pushed the gakonst-asset-scale branch from 4927f5d to d227963 Compare August 8, 2019 15:57

gakonst force-pushed the gakonst-precision-loss branch from 09c89e6 to 0b8ee3f Compare August 9, 2019 09:24

gakonst commented Aug 9, 2019

View reviewed changes

Comment thread crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs Outdated

emschwartz requested changes Aug 9, 2019

View reviewed changes

gakonst added 2 commits August 9, 2019 20:59

feature(engines): Add trait for saving leftover values

e892efa

Implement the trait for the eth engine

feat(engine): Add leftovers value when settling to connector

055d365

gakonst force-pushed the gakonst-precision-loss branch from 05dce10 to 055d365 Compare August 9, 2019 18:01

gakonst changed the base branch from gakonst-asset-scale to master August 9, 2019 18:01

gakonst added 4 commits August 9, 2019 21:26

fix(engines): Make leftovers be an associated type

68da282

This allows the trait implementer to choose the data type of the leftovers instead of forcing them to use BigUint

fix(engine-store): load_leftovers -> pop_leftovers + test

dce326d

fix(tests): return 0 if there were no leftovers found in the store

11b99a8

reenable the response processing of the connector

72751ba

gakonst mentioned this pull request Aug 12, 2019

Update the /settlements controller to return a SettlementResponse interledgerjs/settlement-xrp#7

Closed

fix(eth-se): Box most futures to work around type_length_error

88dc75d

Note: https://github.com/interledgerjs/settlement-xrp is not updated yet to return data for checking leftovers

gakonst force-pushed the gakonst-precision-loss branch from 488677e to 88dc75d Compare August 12, 2019 14:03

gakonst requested a review from emschwartz August 12, 2019 14:04

emschwartz requested changes Aug 12, 2019

View reviewed changes

rename leftovers to uncredited_settlement_amount

cf1dbd6

sappenin mentioned this pull request Aug 12, 2019

Start tracking Settlement leftovers interledgerjs/settlement-xrp#11

Closed

use an array of strings to store multiple BigUint uncredited values

19e0cd1

emschwartz reviewed Aug 13, 2019

View reviewed changes

test(eth-engine): ensure that the test works with numbers larger than…

00a30dd

… u64

gakonst requested a review from emschwartz August 13, 2019 17:13

emschwartz approved these changes Aug 14, 2019

View reviewed changes

gakonst added 2 commits August 15, 2019 19:31

Merge branch 'master' into gakonst-precision-loss

27e451f

chore: prefix uncredited amount key

0d5a477

gakonst force-pushed the gakonst-precision-loss branch from db28493 to 0d5a477 Compare August 15, 2019 16:50

gakonst merged commit 7cfa6bf into master Aug 15, 2019

gakonst deleted the gakonst-precision-loss branch August 15, 2019 17:04

sappenin mentioned this pull request Aug 20, 2019

Confirm lua script functionality interledger4j/ilpv4-connector#268

Closed

Uh oh!

Conversation

gakonst commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gakonst commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gakonst commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gakonst commented Aug 12, 2019

Uh oh!

emschwartz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emschwartz Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

gakonst Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emschwartz Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

kincaidoneil Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

gakonst Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

emschwartz Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

emschwartz Aug 14, 2019

Choose a reason for hiding this comment

Uh oh!

emschwartz Aug 14, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gakonst commented Aug 8, 2019 •

edited

Loading

gakonst commented Aug 9, 2019 •

edited

Loading

gakonst commented Aug 12, 2019 •

edited

Loading

gakonst Aug 12, 2019 •

edited

Loading